Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effort limits for Kinematic Trajectory Optimization #22579

Merged

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Feb 1, 2025

Resolves #22500.

+@hongkai-dai for feature review, please?

Note: I've included PR #22552 as the first commit here; the request is only to review the second commit.


This change is Reviewable

@RussTedrake RussTedrake added the release notes: feature This pull request contains a new feature label Feb 1, 2025
Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 7 files at r2.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 383 at r2 (raw file):

    KinematicTrajectoryOptimization kto(plant->num_positions(),
                                        num_control_points);
    kto.AddDurationConstraint(1, 1);

Would it be better to use a duration not equal to 1? Using duration equal to 1 would not test whether we have re-scaled the velocity/acceleration with duration in our code.


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 394 at r2 (raw file):

  // Variables for the constraint are [duration, control_points]
  VectorXd x(1 + num_control_points);
  const double duration = 1.0;

Ditto.


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 396 at r2 (raw file):

  const double duration = 1.0;
  auto vdot_ulimit = [&](double q, double v) {
    // ml^2vdot + b*v + mglsin(q) <= 1.0

The right hand side is tau_ub instead of 1.0?


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 200 at r2 (raw file):

 public:
  EffortConstraint(const std::shared_ptr<MultibodyPlant<AutoDiffXd>>& plant,
                   std::unique_ptr<systems::Context<AutoDiffXd>> plant_context,

Curious why we use a unique ptr of plant_context here. Do you think we will have other constraints in the future, that will also use the plant_context that evaluate at s? If so, maybe it is better to share the plant_context pointer to reuse the cache?


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 210 at r2 (raw file):

    // Note: consistency checks on the input arguments already happened in
    // AddEffortBoundsAtNormalizedTimes().
    M_q_ = bspline.EvaluateLinearInControlPoints(s, 0).cast<AutoDiffXd>();

When I first saw the variable name M_vdot_, I thought it means the inertial matrix M times vdot.

Maybe add documentation to clarify it

M_q_, M_v_, M_vdot_ are matrices such that by multiplying control_points with these matrics, we get the position q, the derivative of q w.r.t s, and the second derivative of q w.r.t s respectively.

planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 228 at r2 (raw file):

    MatrixX<AutoDiffXd> control_points =
        x.tail(plant_->num_positions() * num_control_points_)
            .reshaped(plant_->num_positions(), num_control_points_);

Do we need to set the time of the plant_context_ to be s * duration


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 576 at r2 (raw file):

  DRAKE_THROW_UNLESS(effort_lb.size() == num_positions());
  DRAKE_THROW_UNLESS(effort_ub.size() == num_positions());
  DRAKE_THROW_UNLESS((effort_lb.array() <= effort_ub.array()).all());

I thought we want to check if B * effort_lb <= B * effort_ub?

Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 unresolved discussions, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 200 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Curious why we use a unique ptr of plant_context here. Do you think we will have other constraints in the future, that will also use the plant_context that evaluate at s? If so, maybe it is better to share the plant_context pointer to reuse the cache?

Done. I like that. (but do we agree that I should make separate contexts for each constraint in add method?)


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 210 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

When I first saw the variable name M_vdot_, I thought it means the inertial matrix M times vdot.

Maybe add documentation to clarify it

M_q_, M_v_, M_vdot_ are matrices such that by multiplying control_points with these matrics, we get the position q, the derivative of q w.r.t s, and the second derivative of q w.r.t s respectively.

Done.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 228 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Do we need to set the time of the plant_context_ to be s * duration

Done. Good call. I don't know that we have any time-dependent things in MbP, but you're right that we should do this in case.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 576 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I thought we want to check if B * effort_lb <= B * effort_ub?

Done. Thanks! (I thought through it carefully when i wrote the doc string, but then forgot when i did the implementation 🤦 )


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 383 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Would it be better to use a duration not equal to 1? Using duration equal to 1 would not test whether we have re-scaled the velocity/acceleration with duration in our code.

Done. (it was complicated to do that before I switched to calling a small kintrajopt to make the control points!)


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 394 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Ditto.

Done.


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 396 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

The right hand side is tau_ub instead of 1.0?

Done.

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: the CI failure is real. +@xuchenhan-tri for platform review please, thanks!

Reviewed 5 of 7 files at r4.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 200 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. I like that. (but do we agree that I should make separate contexts for each constraint in add method?)

Yes, I agree we need separate contexts for each constraint.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable is a little messed up for me here. Which revision should I focus on here?

Reviewed 7 of 7 files at r1, 7 of 7 files at r3.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)


a discussion (no related file):
FYI, #22552 is now merged. I'll loop back to this PR when it's rebased and the unit tests are fixed.

@mmmspatz
Copy link

mmmspatz commented Feb 4, 2025

@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please

@RussTedrake RussTedrake force-pushed the kintrajopt_effort branch 2 times, most recently from 1e2a02f to 59ef4ad Compare February 8, 2025 15:11
Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to go. PTAL. I added more tests to c++ and resolved the (real) error that I had with underactuated systems.

Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Platform :lgtm: with minor comments.

Reviewed 2 of 7 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: 6 unresolved discussions


planning/trajectory_optimization/kinematic_trajectory_optimization.h line 224 at r5 (raw file):

  Pass `plant_context` if you have non-default parameters in the context. Note
  that there are no lifetime requirements on `plant` nsor `plant_context`.

typo

Suggestion:

  that there are no lifetime requirements on `plant` nor `plant_context`.

planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 205 at r5 (raw file):

class EffortConstraint : public Constraint {
 public:
  EffortConstraint(const std::shared_ptr<MultibodyPlant<AutoDiffXd>>& plant,

BTW, some documentation on the constructor would be nice.

Also, it's not clear to me why the plant context uses shared ownership (instead of unique ownership).


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 232 at r5 (raw file):

              AutoDiffVecXd* y) const override {
    y->resize(plant_->num_velocities());
    unused(x);

Is the unused(x) here intentional? It is used in the line below.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 236 at r5 (raw file):

    MatrixX<AutoDiffXd> control_points =
        x.tail(plant_->num_positions() * num_control_points_)
            .reshaped(plant_->num_positions(), num_control_points_);

BTW, these can be const.

Code quote:

    AutoDiffXd duration = x[0];
    MatrixX<AutoDiffXd> control_points =
        x.tail(plant_->num_positions() * num_control_points_)
            .reshaped(plant_->num_positions(), num_control_points_);

planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 263 at r5 (raw file):

  VectorX<AutoDiffXd> M_qdot_;
  VectorX<AutoDiffXd> M_qddot_;
  int num_control_points_;

nit, default initialization for primitive types.


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 449 at r5 (raw file):

  EXPECT_EQ(bindings[0].evaluator()->lower_bound(), tau_lb);
  EXPECT_EQ(bindings[0].evaluator()->upper_bound(), tau_ub);
  // The torque that tripped do to high damping is now satisfied.

typo

Suggestion:

 // The torque that tripped due to high damping is now satisfied.

Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 205 at r5 (raw file):

Previously, xuchenhan-tri wrote…

BTW, some documentation on the constructor would be nice.

Also, it's not clear to me why the plant context uses shared ownership (instead of unique ownership).

Done. Re: shared ownership (Because the feature viewer requested it. ;-). In this code we are only using unique ownership, but we could imagine using more one-context-per-thread workflows in the future.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 232 at r5 (raw file):

Previously, xuchenhan-tri wrote…

Is the unused(x) here intentional? It is used in the line below.

Done. thanks. that was left over from a debugging session.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 236 at r5 (raw file):

Previously, xuchenhan-tri wrote…

BTW, these can be const.

Done.


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 263 at r5 (raw file):

Previously, xuchenhan-tri wrote…

nit, default initialization for primitive types.

Done.


planning/trajectory_optimization/test/kinematic_trajectory_optimization_test.cc line 449 at r5 (raw file):

Previously, xuchenhan-tri wrote…

typo

Done.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees hongkai-dai,xuchenhan-tri(platform)


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 205 at r5 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. Re: shared ownership (Because the feature viewer requested it. ;-). In this code we are only using unique ownership, but we could imagine using more one-context-per-thread workflows in the future.

The change here didn't come through.

I didn't understand why using a shared ptr to context enables the one-context-per-thread work flow. Consider briefly explaining the intention in the documentation of the constructor.

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r5, 2 of 3 files at r6.
Reviewable status: 1 unresolved discussion

Copy link
Contributor Author

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion


planning/trajectory_optimization/kinematic_trajectory_optimization.cc line 205 at r5 (raw file):

Previously, xuchenhan-tri wrote…

The change here didn't come through.

I didn't understand why using a shared ptr to context enables the one-context-per-thread work flow. Consider briefly explaining the intention in the documentation of the constructor.

Done.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees hongkai-dai,xuchenhan-tri(platform)

@xuchenhan-tri xuchenhan-tri merged commit 9b77770 into RobotLocomotion:master Feb 9, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Effort limits for KinematicTrajectoryOptimization
4 participants